Allow building the image without needing to launch it #1647

consideRatio · 2023-03-24T08:05:03Z

(PR written and made by @GeorgianaElena, it was just initialized by @consideRatio to get a branch to reference)

This PR adds a feature that offers the option of an API only mode, and in this mode, it allows opting-out of launching directly after build.

It adds:

a traitlet called enable_api_only_node that defaults to False to preserve existing default behavior. When enabled, there will be no UI, with the only registered endpoints being:
- /metrics
- /versions
- /build
- /health
- /_config
- /* -> shows a custom 404 page
a query parameter called build_only to be passed to individual requests to enable the build only only when enable_api_only_node is True

flowchart LR
  subgraph API only mode enabled
     direction TB
       id1{{false}}
       id2{{false}}
       id3{{false}}
       id4{{true}}
       id5{{true}}
       id6{{true}}
  end

  subgraph `build_only` query param
     direction TB
     id1 ---|and|id11{{missing}}
     id2 ---|and|id12{{false}}
     id3 ---|and|id13{{true}}
     id4 ---|and|id14{{missing}}
     id5 ---|and|id15{{false}}
     id6 ---|and|id16{{true}}
  end

   subgraph Outcome
     direction TB
     id11 ==> id21{{OK, image will be launched after build}}
     id12 ==> id22{{OK, image will be launched after build}}
     id13 ==> id23{{ERROR, building but not launching is not permitted when API only mode is not enabled}}
     id14 ==> id24{{OK, image will be launched after build}}
     id15 ==> id25{{OK, image will be launched after build}}
     id16 ==> id26{{OK, image will NOT be launched after build}}
  end

tests
- for the configuration corner cases that generate errors
- for the actual build only config

It also updates the auth tests for consistency to expect a more descriptive pytest request parameter value in order for the app fixture to load the extra auth configuration. This is for consistency with the build only extra config that was added in this PR.

Todo

Document this once there's agreement on naming an evaluation of the the traitlet and query parameter values to come to an outcome.

Background

This is meant as a building block for the https://github.com/2i2c-org/binderhub-service project. In brief, its positioning itself to be relevant specifically for situations when a binderhub UI is relevant together with a JupyterHub behind authentication and that also provides user home folder storage.

yuvipanda

Thank you so much for working on this, @GeorgianaElena!

In thinking about config, I was originally thinking about two traitlets and a query param, and made this table:

Scenario	`allow_no_launch` traitlet	`require_no_launch` traitlet	`launch=false` query parameter
mybinder.org / public binders	false	false	error is returned, saying 'building but not launching is not permitted'
binderhub used as a service to build images for a JupyterHub	true	true	works as intended
Mysterious scenario where a single binderhub is used both to launch into a preconfigured JupyterHub but also as an API for just building images	true	false	works as intended

However, upon looking at this table now, I think we don't really want to support row 3 - I don't see a good use case for that. So instead, I think the table should be:

Scenario	`require_no_launch` traitlet	`launch=false` query parameter
mybinder.org / public binders	false	error is returned, saying 'building but not launching is not permitted'
binderhub used as a service to build images for a JupyterHub	true	works as intended. If `launch=false` is not specified, return an error

So in effect, to deploy a binderhub that will not try to launch into an existing JupyterHub automatically, the following things must both happen:

The binderhub Admin must set this traitlet config to True, stating their intention that they do not want to launch anywhere. This also allows them to skip the config for connecting a JupyterHub to this.
The person building the frontend code that makes this API call must also set a query parameter, stating their intention to just get back a built image spec - and not want an automatic launch.

I think the current combination of traitlets and query parameters is close, but doesn't quite implement this. Currently, the query parameter just overrides the traitlet - so the image gets built and not launched if either the traitlet or the query param is true. For public facing binders like mybinder.org, I don't think we want to allow this behavior - we want to allow image building but not launching if both the traitlet and the query parameter is true. Switch from an or to an and.

I hope this makes sense! Let me know if it does not, and I can try clarify.

GeorgianaElena · 2023-04-07T10:53:46Z

I think I understand @yuvipanda. Thanks for the explanation. Can you double check the following points to make sure I got it right, please?

Double check list

When require_no_launch = False, then a mybinder scenario is considered, meaning that the query can be:

missing, in which case everything remains unchanged
launch=true, in which case everything remains unchanged
launch=false, in which case error is returned, saying 'building but not launching is not permitted'

When require_no_launch = True, then a service-like scenario is considered, meaning that the query can be:

missing, in which case return an error
launch=true, in which case we launch?? (don't sure about this one. Do we still want an and situation herre also?)
launch=false, in which case we don't launch

Prior discussions

Currently, the query parameter just overrides the traitlet - so the image gets built and not launched if either the traitlet or the query param is true.

I believe @consideRatio made a point about why this (overriding through the query the traitlet) might be useful, but don't remember what it was 😬 @consideRatio wdyt?

Naming

Thinking more about the naming, I realize that having a traitlet with a negation in its name, and then the query without, it's kind of confusion. Also, checking if no require_no_launch is also confusing I believe.

What do you think about a:

traitlet called allow_build_only if we want to allow image building only if both the traitlet and the query parameter are try true
query param called build_only

yuvipanda · 2023-04-07T10:55:37Z

launch=true, in which case we launch?? (don't sure about this one. Do we still want an and situation herre also?)

I think in this case, we return an error - launch=false is required. This allows the binderhub to exist without requiring a JupyterHub it has launch rights on to be configured.

I like your proposed build based naming scheme, we can go with that. Thank you for thinking that through!

GeorgianaElena · 2023-04-07T13:01:17Z

@yuvipanda, I've updated the PR if you want to take another look

consideRatio

Thank you for working on this @GeorgianaElena and @yuvipanda!!

binderhub/tests/test_build.py

binderhub/builder.py

GeorgianaElena · 2023-04-21T12:50:29Z

Thanks for the feedback @consideRatio ✨ it helped me simplify and correct the logic a bit more while also keeping the old log messages. I've also managed to get an understanding of the current tests and added some for the new functionality.

This is why I marked this as ready for review.

Note that it still lacks documentation but I plan to add that once there's agreement on the implementation.

P.S. thanks @colliand for showing off https://github.com/mermaid-js/mermaid in another PR. This was used to create the diagram in the top comment #1647 (comment) :D

consideRatio · 2023-04-24T00:21:21Z

Thank you for working this @GeorgianaElena!!! I'll look at this tomorrow/monday morning!

consideRatio

Great work on this @GeorgianaElena!!!

binderhub/builder.py

binderhub/app.py

binderhub/tests/conftest.py

binderhub/tests/test_build.py

consideRatio

This looks great to me @GeorgianaElena!

With this well documented traitlet for an opt-in feature only to be used in certain circumstances, I consider this mergable as it is.

GeorgianaElena · 2023-04-28T08:11:11Z

After testing this out with @consideRatio yesterday, we concluded that even though we decided that it is out of scope of this PR to support a new UI for the build only option, it is confusing to still have the classical build + launch available.

When clicking the launch button, that will imediatlly fail with:

With the last commit, b82758e, when require_build_only is set, the classical UI is deactivated and instead a 204 No content page is rendered using the error template. It looks like below:

Does something like this makes sense?

GeorgianaElena · 2023-04-28T13:21:04Z

Failures seem to be happening on the main branch as well https://github.com/jupyterhub/binderhub/actions/runs/4829259002/jobs/8604077929 so not sure which if any are related to the PR

consideRatio · 2023-04-28T13:23:45Z

@GeorgianaElena I've been debugging this for quite some time now, I'm drawing blank.

Apparently the hub pods launching are stuck pending without k8s Events associated with them. That indicates to me they aren't getting scheduled, not even failing to get scheduled to a node I think - but why?

GeorgianaElena · 2023-04-28T14:06:18Z

Just added a commit to not register any handlers for / and /v2 when require_build_only is True instead to keep it simple and secure as per discussion with @consideRatio

consideRatio

I'm still brainstorming not confident on what I think is desirable

Do we want to de-register /about as well, and possibly /_config that I don't know what it does? And along with that perhaps deregistering everything about favicons and badges etc?
Should Custom404 respond with JSON instead if require_build_only is set?
If require_build_only pretty much is "api only mode", should we make api_only_mode a standalone configuration or similar? Hmmm, this is a jupyterhub thing as well i recall... Yes https://jupyterhub.readthedocs.io/en/latest/howto/api-only.html writes about an API Only mode.

binderhub/app.py

consideRatio

This looks great to me!

I think that the prometheus metrics BUILD_TIME and BUILD_COUNT should be tracked still, let's resolve that and ask someone else to review this after that.

binderhub/builder.py

for more information, see https://pre-commit.ci

GeorgianaElena · 2023-09-22T09:24:30Z

@minrk, I've added a few commits based on your feedback above (thank you),. I've implemented a combination of your suggestions above! What do you think about:

give the config a name that indicates how much is changing, e.g. build_only_api_only or disable_launch_and_ui

accept the only allowed value when unspecified in both cases, rather than requiring opt-in to the only accepted value (I’m less sure about this, especially in more detailed proposal below)

enable_api_only_mode config
This introduces an API mode that only keeps the /build endpoint and a few other that are informative, like metrics, versions, health etc.

Without additional config, this mode does not assume anything about a build only behavior
build_only query parameter

This flag enables build only when it's set to True. In all other cases, the /build endpoint will do what it does by default, which is to build + launch.

⬆️ This is what I am unsure of. Does an api only mode only makes sense in a build only scenario?

minrk

Sorry for leaving you waiting so long, it's a busy Fall! I left some minor comments to address in naming and clarity, but I think this is solid as is, and feel free to merge and iterate on refinements in smaller later PRs.

Does an api only mode only makes sense in a build only scenario?

I don't think so. A separate service could use an api-only BinderHub to build and launch and then redirect to the running server. An example would be a service that uses BinderHub, but presents its own UI to the user before redirecting to a running Jupyter server. Much like BinderHub itself uses JupyterHub in api-only mode.

minrk · 2023-10-10T07:51:37Z

binderhub/builder.py

+        build_only_outcome = False
+        if build_only_query_parameter.lower() == "true":
+            if not enable_api_only_mode:
+                raise HTTPError(


This lacks a status code. Presumably it should be 400?

minrk · 2023-10-10T07:52:18Z

binderhub/builder.py

@@ -228,6 +228,24 @@ def set_default_headers(self):
        self.set_header("content-type", "text/event-stream")
        self.set_header("cache-control", "no-cache")

+    def _get_build_only_outcome(self):


I'd call this _get_build_only(), since it's the dynamic resolution of the value of build_only with checks, etc., not a separate concept called 'outcome', which confused me a bit.

minrk · 2023-10-10T07:54:03Z

binderhub/builder.py

@@ -408,33 +426,52 @@ async def get(self, provider_prefix, _unescaped_spec):
            else:
                image_found = True

-        if image_found:
+        build_only_outcome = self._get_build_only_outcome()


Same here, let's call it build_only, which I think is clearer than adding outcome

… http error Co-authored-by: Min RK <benjaminrk@gmail.com>

for more information, see https://pre-commit.ci

GeorgianaElena · 2023-10-10T09:42:44Z

Thank you very much @minrk for the review and suggestions. I've added a commit that addresses them.

Some tests seem to be failing now though, but the same are failing for the main branch as well, so I don't think they are related to this PR?

minrk · 2023-10-10T09:50:23Z

Yes, failures appear to be helm related, not related to this PR (one image pull failure and one memory limit issue). Going to merge this one!

jupyterhub/binderhub#1647 Merge pull request #1647 from consideRatio/opt-out-of-launch

consideRatio marked this pull request as draft March 24, 2023 08:05

consideRatio added the new label Mar 24, 2023

consideRatio mentioned this pull request Mar 24, 2023

Create a fork of binderhub to develop opt-out feature of not launching following build/push 2i2c-org/binderhub-service#6

Closed

consideRatio force-pushed the opt-out-of-launch branch from 2e7b75e to d4b83c0 Compare March 24, 2023 12:03

consideRatio mentioned this pull request Apr 3, 2023

Draft plan of work towards a standalone demo 2i2c-org/binderhub-service#15

Closed

GeorgianaElena self-assigned this Apr 6, 2023

GeorgianaElena force-pushed the opt-out-of-launch branch from d4b83c0 to 4eae8ee Compare April 6, 2023 10:00

yuvipanda requested changes Apr 7, 2023

View reviewed changes

GeorgianaElena force-pushed the opt-out-of-launch branch from b3663ca to 8b64b1b Compare April 7, 2023 12:56

consideRatio commented Apr 18, 2023

View reviewed changes

GeorgianaElena force-pushed the opt-out-of-launch branch from 0ce266d to 9ed19c4 Compare April 21, 2023 11:50

GeorgianaElena marked this pull request as ready for review April 21, 2023 12:46

consideRatio commented Apr 24, 2023

View reviewed changes

This was referenced Apr 25, 2023

Live notes towards a test deployment and demo 2i2c-org/binderhub-service#19

Closed

Release planning for 1.0.0 #1667

Open

GeorgianaElena force-pushed the opt-out-of-launch branch from 1fcb6d4 to b82758e Compare April 28, 2023 07:58

consideRatio commented Apr 28, 2023

View reviewed changes

binderhub/app.py Outdated Show resolved Hide resolved

consideRatio mentioned this pull request Apr 28, 2023

Overview of future work 2i2c-org/binderhub-service#27

Open

minrk changed the title ~~Opt-in feature to allow opting out of launching following build~~ Opt-in feature to allow opting out of launch following build May 11, 2023

consideRatio commented Jun 15, 2023

View reviewed changes

binderhub/builder.py Outdated Show resolved Hide resolved

binderhub/builder.py Outdated Show resolved Hide resolved

pre-commit-ci bot and others added 12 commits September 20, 2023 14:02

[pre-commit.ci] auto fixes from pre-commit.com hooks

a7d2418

for more information, see https://pre-commit.ci

Just deactivate the / and /v2 endpoints when require_build_only is True

252987c

[pre-commit.ci] auto fixes from pre-commit.com hooks

0203a2d

for more information, see https://pre-commit.ci

Update build success metrics even if we don't launch after

15dc9d3

[pre-commit.ci] auto fixes from pre-commit.com hooks

f3d174b

for more information, see https://pre-commit.ci

Fix indentation

c20ca53

Replace require_build_only for a more clear enable_api_only_mode config

f7a4c3c

Update how we decide if build only mode was activated

5cf641d

Get the Custom404 handler in both cases

61d1a5d

update message

08d4f1d

Update the test config

368cad7

Update tests

693baf9

GeorgianaElena force-pushed the opt-out-of-launch branch from 5c201d1 to 693baf9 Compare September 21, 2023 14:45

pre-commit-ci bot and others added 2 commits September 21, 2023 14:45

[pre-commit.ci] auto fixes from pre-commit.com hooks

b2925dc

for more information, see https://pre-commit.ci

Updat log message and assert

6252eb6

yuvipanda mentioned this pull request Oct 4, 2023

Allow passing additional args to repo2docker #1765

Closed

minrk approved these changes Oct 10, 2023

View reviewed changes

GeorgianaElena and others added 2 commits October 10, 2023 11:30

Drop the outcome suffix for clariry and specify a status code for the…

114b195

… http error Co-authored-by: Min RK <benjaminrk@gmail.com>

[pre-commit.ci] auto fixes from pre-commit.com hooks

d5f686d

for more information, see https://pre-commit.ci

minrk merged commit 0dde0c0 into jupyterhub:main Oct 10, 2023
11 of 13 checks passed

consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request Oct 10, 2023

[binderhub] Automatic update for commit 1.0.0-0.dev.git.3290.h0dde0c0

1339685

jupyterhub/binderhub#1647 Merge pull request #1647 from consideRatio/opt-out-of-launch

jupyterhub-bot mentioned this pull request Oct 11, 2023

Updates binderhub chart to 1.0.0-0.dev.git.3298.hfe2bf1b jupyterhub/mybinder.org-deploy#2771

Closed

consideRatio mentioned this pull request Oct 12, 2023

Allow passing buildOnly param to the server from the js client #1742

Merged

jupyterhub-bot mentioned this pull request Oct 14, 2023

Updates binderhub chart to 1.0.0-0.dev.git.3308.h4a92ad9 jupyterhub/mybinder.org-deploy#2775

Merged

manics mentioned this pull request Dec 10, 2023

local-binder-mocked-hub: undo api_only_mode #1803

Merged

jtpio mentioned this pull request Jan 19, 2024

UI for building the environments 2i2c-org/binderhub-service#78

Open

yuvipanda mentioned this pull request Jun 6, 2024

Add repo2docker frontend feature FZJ-JSC/jupyter-jsc-share#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow building the image without needing to launch it #1647

Allow building the image without needing to launch it #1647

consideRatio commented Mar 24, 2023 •

edited by GeorgianaElena

yuvipanda left a comment

GeorgianaElena commented Apr 7, 2023

yuvipanda commented Apr 7, 2023

GeorgianaElena commented Apr 7, 2023

consideRatio left a comment

GeorgianaElena commented Apr 21, 2023 •

edited

consideRatio commented Apr 24, 2023 •

edited

consideRatio left a comment

consideRatio left a comment

GeorgianaElena commented Apr 28, 2023

GeorgianaElena commented Apr 28, 2023

consideRatio commented Apr 28, 2023

GeorgianaElena commented Apr 28, 2023

consideRatio left a comment

consideRatio left a comment

GeorgianaElena commented Sep 22, 2023

minrk left a comment

minrk Oct 10, 2023

minrk Oct 10, 2023

minrk Oct 10, 2023

GeorgianaElena commented Oct 10, 2023

minrk commented Oct 10, 2023

Allow building the image without needing to launch it #1647

Allow building the image without needing to launch it #1647

Conversation

consideRatio commented Mar 24, 2023 • edited by GeorgianaElena

Todo

Background

yuvipanda left a comment

Choose a reason for hiding this comment

GeorgianaElena commented Apr 7, 2023

Double check list

Prior discussions

Naming

yuvipanda commented Apr 7, 2023

GeorgianaElena commented Apr 7, 2023

consideRatio left a comment

Choose a reason for hiding this comment

GeorgianaElena commented Apr 21, 2023 • edited

consideRatio commented Apr 24, 2023 • edited

consideRatio left a comment

Choose a reason for hiding this comment

consideRatio left a comment

Choose a reason for hiding this comment

GeorgianaElena commented Apr 28, 2023

GeorgianaElena commented Apr 28, 2023

consideRatio commented Apr 28, 2023

GeorgianaElena commented Apr 28, 2023

consideRatio left a comment

Choose a reason for hiding this comment

consideRatio left a comment

Choose a reason for hiding this comment

GeorgianaElena commented Sep 22, 2023

minrk left a comment

Choose a reason for hiding this comment

minrk Oct 10, 2023

Choose a reason for hiding this comment

minrk Oct 10, 2023

Choose a reason for hiding this comment

minrk Oct 10, 2023

Choose a reason for hiding this comment

GeorgianaElena commented Oct 10, 2023

minrk commented Oct 10, 2023

consideRatio commented Mar 24, 2023 •

edited by GeorgianaElena

GeorgianaElena commented Apr 21, 2023 •

edited

consideRatio commented Apr 24, 2023 •

edited